Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(ci): added continuous integration tests #4577

Merged
merged 21 commits into from
Jul 26, 2024

Conversation

Sebastian-Webster
Copy link
Contributor

@Sebastian-Webster Sebastian-Webster commented Jul 19, 2024

Added CI tests

Before this PR there were CI tests, but they were only allowed to run on the master branch, and this repository does not have a master branch so they never ran. The changes in this PR are as follows:

  1. CI now uses updated versions of Node.js, 18.x, 20.x, 22.x
  2. CI only runs on pull requests to main instead of both pushes and pull requests. I considered having them stay running on both, but if a push to main makes a test fail, the next commit fixing the fail would make the tests fail again as they differ from the previous commit. It's an easy change if it's desired for tests to run on pushes as well though.
  3. The linting step is now in its own workflow file. This was done to seperate it from tests so a lint error doesn't cause the tests to fail. The linting workflow runs on both pushes and pull requests to main.
  4. The testing workflow now has 2 jobs. One for testing & building, and the other to test that the changes are reproducible.
  5. When a new test is added and a snapshot is missing, instead of throwing an error, a warning is now displayed saying that the test is missing a snapshot. This will prevent PRs that add new tests to have their CI jobs fail from a missing snapshot.
  6. A new test:ci script was made so tests that fail in CI will exit with an error status code, causing the CI job to fail. This was done because the npm test command does not exit with an error status code when a test fails, so if using the normal test command, a CI job will get marked as successful even if the tests fail.
  7. A checkbox was added to the pull request template to remind people to merge main with their branches. If a change in main makes the snapshots look different, the CI job will fail if the changes aren't also present in the PR branch.

The testing & building workflow runs steps for 2 different repositories. One for the base repository (the apexcharts repository) and the head repository (repository where the PR code is coming from). The workflow first gets the code from the base repository and generates e2e test snapshots. After that, the head repository is checked out, and tests are ran against the snapshots generated from the base repository.

The reproducibility test is done to make sure any test changes are reproducible.

Base repository samples & snapshots and head repository samples, diffs, coverage, and build files are all uploaded to the workflow artifacts so if a job does fail, it can be easy to take a look at what happened and debug. For any changes that will intentionally fail tests (like an intentional UI change) the snapshots from the artifacts can be looked at to make sure everything looks as expected before merging a pull request.

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • New and existing unit tests pass locally with my changes
  • My branch is up to date with any changes from the main branch

@Sebastian-Webster
Copy link
Contributor Author

Sebastian-Webster commented Jul 19, 2024

When I was working on this PR, the latest version of Node.js at the time (22.5.0) had a regression that caused npm to fail, see nodejs/node#53902 and npm/cli#7672 so I pinned the node-version for Node 22 to 22.4.1 to prevent that regression from causing issues. As I was writing this PR, Node.js 22.5.1 came out and fixes that issue so this PR has gone back to using 22.x (see nodejs/node#53902 (comment))

EDIT: It seems like GitHub Actions has not started using 22.5.1 as the latest for 22.x so 22.4.1 will be used for now until Node gets updated for GitHub Actions

EDIT 2: GitHub Actions node 22.x is now using 22.5.1 (see actions/node-versions#182) so this PR now uses 22.x.

@Sebastian-Webster Sebastian-Webster marked this pull request as draft July 20, 2024 10:40
@Sebastian-Webster Sebastian-Webster marked this pull request as ready for review July 24, 2024 04:18
@junedchhipa
Copy link
Contributor

Fantastic! This is a great addition to the workflow. Thanks @Sebastian-Webster

@junedchhipa junedchhipa merged commit 45afd73 into apexcharts:main Jul 26, 2024
7 checks passed
@Sebastian-Webster
Copy link
Contributor Author

Thank you! No problem, I'm happy to contribute @junedchhipa :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants